src: enable V8's WASM trap handlers by devsnek · Pull Request #27246 · nodejs/node

devsnek · 2019-04-15T21:12:27Z

This uses SIGSEGV handlers to catch WASM out of bound (OOB) memory
accesses instead of inserting OOB checks inline, resulting in a 25%-30%
speed increase.

Note that installing a custom SIGSEGV handler will break this, resulting
in potentially scary behaviour. Users should use node::RegisterSignalHandler instead.

Refs: #14927

Closes #14927

Checklist

make -j4 test (UNIX), or vcbuild test (Windows) passes
commit message follows commit guidelines

ghost · 2019-04-15T21:12:29Z

Lite-CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/3265

ghost · 2019-04-15T21:20:06Z

My gut feeling would be that @ofrobots’s comment in #14927 (comment) is a good approach – using our own signal handler, and providing a public C++ API for forwarding that to addons/embedders? That would mean that those would not have to carry the responsibility of integrating with V8’s APIs.

I’d say this is semver-major because of the conflicts with existing published versions of segfault-handler – do you agree? If yes, then I’m happy with any approach, because we should have more than enough time to update that package.

ghost · 2019-04-15T21:26:10Z

@addaleax we already have node::RegisterSignalHandler, i guess i could set this up with that, although i think this then becomes uh... not platform independent.

we'll need to add a bunch of platform specific switches for calling TryWasmTrapHanderPosix vs TryWasmTrapHandlerWin but if that complexity is okay then we can go for it.

ghost · 2019-04-15T22:03:42Z

@addaleax set up to use RegisterSignalHandler, unfortunately we only have that for posix atm, so maybe we can expand to win in the future? (although i don't think people on windows are expecting great performance anyway 🤷‍♂️)

ghost · 2019-04-15T22:54:24Z

@devsnek Yeah, we’d need to find some common API (or expose 2 variants) for both Windows and POSIX; and maybe find some API constraints, e.g. what happens if it is called multiple times, is the registering function threadsafe, etc. and document that

ghost · 2019-04-16T06:14:36Z

Can you put this into the per_process namespace and make it static?

ghost · 2019-04-16T09:07:06Z

Sorry, I forgot to mention something important: you must re-raise the SIGSEGV when V8 doesn't handle it and there's no other handler for it.

ghost · 2019-04-30T07:20:28Z

Not a blocker, just an observation, but I expect that's no longer relevant for the freebsd versions we support.

@nodejs/platform-freebsd can anyone confirm or deny?

@bnoordhuis do you know if there's any more background/detail on the issue?

@kostikbel any insight? The referenced comment is:

// FreeBSD has a nasty bug with SA_RESETHAND reseting the SA_SIGINFO, that is // in turn set for a libthr wrapper. This leads to a crash. // Work around the issue by manually setting SIG_DFL in the signal handler

From my copy of POSIX 1003.1-2017 AKA SUSv7:

SA_RESETHAND If set, the disposition of the signal shall be reset to SIG_DFL and the SA_SIGINFO flag shall be cleared on entry to the signal handler.

And we do exactly that. Am I reading right that this behavior causes some troubles, when combined with libthr ? Can you give a minimal example ?

@emaste @kostikbel The bug report is nodejs/node-v0.x-archive#9326, triage starts in nodejs/node-v0.x-archive#9326 (comment).

I'm reasonably sure that bug was fixed in fbsd 11 but you probably know for sure, @kostikbel, since you authored the fix.

Ok, so should this comment (and possibly some code) removed ? Since I believe all supported FreeBSD versions have the SA_RESETHAND | SA_SIGINFO fix.

That sounds about right (removing code & comment.) I've opened #27515 to track it.

ghost · 2019-04-30T13:42:13Z

@bnoordhuis do you know why this compiles fine on macos but has issues finding those atomic symbols on the linux build?

ghost · 2019-05-01T04:56:35Z

Reproducing here because scrolling trough Travis really is quite horrid:

Details

/home/travis/build/nodejs/node/out/Release/obj.target/libnode/src/node.o: In function `node::TrapWebAssemblyOrContinue(int, siginfo_t*, void*) [clone .part.36]':
node.cc:(.text+0xce): undefined reference to `__atomic_load'
/home/travis/build/nodejs/node/out/Release/obj.target/libnode/src/node.o: In function `node::RegisterSignalHandler(int, void (*)(int, siginfo_t*, void*), bool, sigaction*)':
node.cc:(.text+0x2484): undefined reference to `__atomic_exchange'
collect2: error: ld returned 1 exit status
cctest.target.mk:226: recipe for target '/home/travis/build/nodejs/node/out/Release/cctest' failed
make[1]: *** [/home/travis/build/nodejs/node/out/Release/cctest] Error 1
make[1]: *** Waiting for unfinished jobs....
/home/travis/build/nodejs/node/out/Release/obj.target/libnode/src/node.o: In function `node::TrapWebAssemblyOrContinue(int, siginfo_t*, void*) [clone .part.36]':
node.cc:(.text+0xce): undefined reference to `__atomic_load'
/home/travis/build/nodejs/node/out/Release/obj.target/libnode/src/node.o: In function `node::RegisterSignalHandler(int, void (*)(int, siginfo_t*, void*), bool, sigaction*)':
node.cc:(.text+0x2484): undefined reference to `__atomic_exchange'
collect2: error: ld returned 1 exit status

I'm going to guess that gcc doesn't know how to lower the __atomic_load and __atomic_exchange builtins for compound types (i.e., struct sigaction) and just re-emits them as library calls.

If you switch to function pointers, the problem should hopefully go away.

ghost · 2019-05-01T07:52:19Z

Or maybe link to libatomic? https://gcc.gnu.org/bugzilla/show_bug.cgi?id=65756#c1

ghost · 2019-05-16T16:19:36Z

CI: https://ci.nodejs.org/job/node-test-pull-request/23146/

This uses SIGSEGV handlers to catch WASM out of bound (OOB) memory accesses instead of inserting OOB checks inline, resulting in a 25%-30% speed increase. Note that installing a custom SIGSEGV handler will break this, resulting in potentially scary behaviour. Refs: nodejs#14927 PR-URL: nodejs#27246 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com> Reviewed-By: Anna Henningsen <anna@addaleax.net>

ghost · 2019-05-16T17:34:46Z

landed in 70c7686

ghost · 2019-05-21T09:43:57Z

@devsnek @addaleax is there a way to backport this PR with a compatibility patch to remove the breaking change? It would already be helpful if e.g. 50% of the code would be backported.

ghost · 2019-05-21T10:52:52Z

@BridgeAR I guess it would be possible to backport this in a non-breaking manner, but it should not be released without #27775 as well

ghost · 2019-05-21T11:00:31Z

@addaleax sounds good 👍. Would you be willing to open the backport after #27775 landed?

ghost · 2019-09-09T17:37:35Z

Notable change:

This speeds up memory operations for WASM by around 25-30%. However, native code must now install the SIGSEGV signal handlers using node::RegisterSignalHandler.

nodejs-github-bot added the c++ Issues and PRs that require attention from people who are familiar with C++. label Apr 15, 2019

devsnek requested review from addaleax, joyeecheung and ofrobots April 15, 2019 21:12

devsnek force-pushed the enable-wasm-trap-handler branch from 2aa0fb8 to 5402f28 Compare April 15, 2019 21:18

devsnek added the semver-major PRs that contain breaking changes and should be released in the next major version. label Apr 15, 2019

devsnek force-pushed the enable-wasm-trap-handler branch 3 times, most recently from ea0ade9 to ab9a5b7 Compare April 15, 2019 22:00

devsnek force-pushed the enable-wasm-trap-handler branch 2 times, most recently from 7c3fee8 to 0865e93 Compare April 15, 2019 22:18

devsnek added v8 engine Issues and PRs related to the V8 dependency. wasm Issues and PRs related to WebAssembly. labels Apr 16, 2019

devsnek commented Apr 16, 2019

View reviewed changes

Comment thread src/node.cc Outdated

devsnek force-pushed the enable-wasm-trap-handler branch from 0865e93 to a244828 Compare April 16, 2019 03:39

joyeecheung reviewed Apr 16, 2019

View reviewed changes

bnoordhuis reviewed Apr 16, 2019

View reviewed changes

Comment thread src/node.cc Outdated

Comment thread src/node.cc Outdated

Comment thread src/node.cc Outdated

Comment thread src/node.cc Outdated

Comment thread src/node.cc Outdated

devsnek force-pushed the enable-wasm-trap-handler branch 2 times, most recently from 94fbe09 to d05e688 Compare April 20, 2019 17:56

devsnek force-pushed the enable-wasm-trap-handler branch from d05e688 to c7d11fe Compare April 30, 2019 05:29

bnoordhuis reviewed Apr 30, 2019

View reviewed changes

devsnek added author ready PRs with CI started, the required approvals, and no outstanding review comments. and removed tsc-agenda Issues and PRs to discuss during Technical Steering Committee meetings. labels May 16, 2019

devsnek force-pushed the enable-wasm-trap-handler branch from 3cdac98 to 70c7686 Compare May 16, 2019 17:33

devsnek merged commit 70c7686 into nodejs:master May 16, 2019

devsnek deleted the enable-wasm-trap-handler branch May 16, 2019 17:35

addaleax mentioned this pull request May 20, 2019

src: fix segfault handling/RegisterSignalHandler #27775

Closed

4 tasks done

bnoordhuis mentioned this pull request May 20, 2019

src: remove freebsd SA_RESETHAND workaround #27780

Closed

BridgeAR added the dont-land-on-v12.x label May 21, 2019

addaleax mentioned this pull request Jun 14, 2019

test: skip test-worker-prof on all platforms. #28175

Closed

BethGriggs mentioned this pull request Sep 9, 2019

2019-10-22, Version 13.0.0 (Current) #29504

Merged

devsnek added the notable-change PRs with changes that should be highlighted in changelogs. label Sep 9, 2019

Conversation

devsnek commented Apr 15, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Checklist

Uh oh!

ghost commented Apr 15, 2019

Uh oh!

ghost commented Apr 15, 2019

Uh oh!

ghost commented Apr 15, 2019 • edited by devsnek Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented Apr 15, 2019

Uh oh!

ghost commented Apr 15, 2019

Uh oh!

Uh oh!

ghost Apr 16, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ghost commented Apr 16, 2019

Uh oh!

ghost Apr 30, 2019

Choose a reason for hiding this comment

Uh oh!

ghost Apr 30, 2019

Choose a reason for hiding this comment

Uh oh!

ghost Apr 30, 2019

Choose a reason for hiding this comment

Uh oh!

ghost Apr 30, 2019 • edited by kostikbel Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost May 1, 2019 • edited by bnoordhuis Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ghost May 1, 2019

Choose a reason for hiding this comment

Uh oh!

ghost May 1, 2019

Choose a reason for hiding this comment

Uh oh!

Uh oh!

ghost commented Apr 30, 2019 • edited by devsnek Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ghost commented May 1, 2019

Uh oh!

ghost commented May 1, 2019

Uh oh!

ghost commented May 16, 2019

Uh oh!

ghost commented May 16, 2019

Uh oh!

ghost commented May 21, 2019

Uh oh!

ghost commented May 21, 2019

Uh oh!

ghost commented May 21, 2019

Uh oh!

ghost commented Sep 9, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

12 participants

devsnek commented Apr 15, 2019 •

edited

Loading

ghost commented Apr 15, 2019 •

edited by devsnek

Loading

ghost Apr 30, 2019 •

edited by kostikbel

Loading

ghost May 1, 2019 •

edited by bnoordhuis

Loading

ghost commented Apr 30, 2019 •

edited by devsnek

Loading